feat(stats): state both goodput ratios, per run and per operation - #79
feat(stats): state both goodput ratios, per run and per operation#79bdchatham wants to merge 2 commits into
Conversation
TOT-005 through TOT-008. Reporting one ratio hides whichever layer it divides away, so the run states both: committed over every send it attempted, and committed over the sends an endpoint accepted. The first answers what the profile asked for. A run whose endpoint refuses half shows a halved number rather than a healthy one measured over the survivors. The second isolates chain execution, which is the number to read when the RPC layer is not under test. The gap between them is the rejection share, so the report names that too. An indeterminate status counts in the denominator and never in the numerator. Counting it in the numerator reports a success nobody saw; dropping it from the denominator reports a healthy ratio over the survivors, which is the defect this feature removes one layer up. Every operation carries its own ledger. The run-level total cannot separate a revert rate spread evenly across operations from one concentrated in a single call, and those point at different things: the chain, or the workload. A run reporting 46% goodput where one operation sits at 0% and another at 98% now says so. It prints as a second line under each operation rather than widening the first, so anything parsing the existing format keeps working. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@seidroid review |
PR SummaryLow Risk Overview Per-operation reporting adds an Tests lock in dual-ratio output, rejection naming, unobserved lowering goodput, separate revert vs expire sections, and safe Reviewed by Cursor Bugbot for commit fede946. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Correctly wires an Attempted denominator and two goodput ratios through the run-level and per-operation reports; the field mapping and the inclusion.Get() hoist that keeps per-op and run-level Tracked in agreement both check out. Two non-blocking gaps: the per-operation line omits the drop and unrecorded counts, so a run whose own registry dropped an operation's transactions prints all zeros and reads as a chain failure, and none of the new tests cover the BuildFinalStats wiring.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The untracked arm of
ExecutionOutcomes.String(stats/execution_outcomes.go:154-161) still reports only "Endpoints accepted %d sends".AttemptedandRejected()are send-path facts that hold whether or not--track-receiptsis on, and by this PR's own argument leaving the reader to subtract "Total TXs" from "accepted" is the arithmetic the feature exists to remove. Stating offered/accepted/rejected there too would cost one line. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
The Attempted denominator, the two goodput ratios, and the per-operation ledger are wired correctly, and the single hoisted inclusion.Get() read keeps run-level and per-op Tracked from disagreeing. No changes have landed since the previous review, so both earlier non-blocking gaps are still open: the per-operation line omits the counts that would explain a zero, and nothing tests the BuildFinalStats wiring.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The untracked arm of
ExecutionOutcomes.String(stats/execution_outcomes.go:154-161) still reports only "Endpoints accepted %d sends".AttemptedandRejected()are send-path facts that hold whether or not--track-receiptsis on, and by this PR's own argument, leaving the reader to subtract "Total TXs" from "accepted" is exactly the arithmetic the feature exists to remove. Stating offered/accepted/rejected there too would cost one line. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
…9-goodput-ratios # Conflicts: # stats/execution_outcomes.go
|
@seidroid review |
There was a problem hiding this comment.
The Attempted denominator, the two goodput ratios and the per-operation ledger are wired correctly, and the hoisted inclusion.Get() keeps run-level and per-op Tracked in agreement. Non-blocking: the new doc comments assert an incorrect relationship between the two ratios (the gap is not the rejection share), the per-operation line omits the counts that would explain a zero, and nothing tests the BuildFinalStats wiring.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The untracked arm of
ExecutionOutcomes.String(stats/execution_outcomes.go:154-161) still reports only "Endpoints accepted %d sends".AttemptedandRejected()are send-path facts that hold whether or not--track-receiptsis on, and by this PR's own argument, leaving the reader to subtract "Total TXs" from "accepted" is exactly the arithmetic the feature exists to remove. Stating offered/accepted/rejected there too would cost one line. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| // Rejected is a send an endpoint refused. It is a fact about the RPC layer, and | ||
| // the gap between the two goodput ratios below is exactly its share. |
There was a problem hiding this comment.
[suggestion] This claim is false in general, and the same claim is repeated at stats/execution_outcomes.go:197-198 and in the PR description.
The gap between the two ratios is
Committed/Accepted - Committed/Attempted
not Rejected/Attempted. The two coincide only when Committed == Accepted, which is exactly the case TestBothRatiosAreStated picks (Attempted: 200, Accepted: 100, Committed: 100 → gap 50%, rejection share 50%), so the test confirms the wrong invariant rather than catching it.
The PR body's own example disproves it: offered 200, accepted 190, goodput 44.00% / 46.32% → gap 2.32%, rejection share 10/200 = 5%.
What actually holds is multiplicative: GoodputOfAttempted = GoodputOfAccepted × (Accepted/Attempted), i.e. the ratio of the two ratios is the acceptance rate. Since these comments are how a reader is told to interpret the report, either state the multiplicative relation or drop the "exactly its share" wording and let rejected %d speak for itself.
| // its readable limit, and anything parsing it keeps working. | ||
| if op.Outcomes.Tracked { | ||
| result += fmt.Sprintf( | ||
| " outcomes: committed=%d reverted=%d expired=%d unobserved=%d | goodput %.2f%% offered, %.2f%% accepted\n", |
There was a problem hiding this comment.
[suggestion] Still open from the previous review — no changes have landed since.
The per-operation line prints four of the seven outcome counts, and the three it drops are the ones that would explain a zero. DroppedAtCap, DroppedAtHandoff and Unrecorded are omitted, so an operation whose transactions this run's own registry refused at cap (stats/inclusion_tracker.go:270) prints:
erc20/transfer: 100 TXs | ...
outcomes: committed=0 reverted=0 expired=0 unobserved=0 | goodput 0.00% offered, 0.00% accepted
Every visible count is zero and goodput is zero, which reads as a chain that committed nothing — the exact false accusation the run-level report is careful to avoid, and which ExecutionOutcomes.String avoids by making no row conditional ("An absent number reads as zero", stats/execution_outcomes.go:143-149). Unrecorded matters for the same reason: it has no legitimate producer, so a per-operation counting bug is invisible in the text report even though the run-level line surfaces it.
Adding dropped=%d (cap + handoff) and unrecorded=%d keeps the line reconcilable against Accepted without widening it much.
| LatencyP99: op.P99Latency, | ||
| SampleCount: op.SampleCount, | ||
| Window: op.Window, | ||
| Outcomes: ExecutionOutcomes{ |
There was a problem hiding this comment.
[suggestion] Still open from the previous review — no test reaches this wiring.
execution_outcomes_test.go exercises Rejected/GoodputOfAttempted/GoodputOfAccepted against struct literals it builds itself, and the only BuildFinalStats caller in the package (stats/logger_order_test.go:72) passes utils.None[stats.InclusionSummary](), so Tracked is false and the per-operation outcomes line never renders. That leaves the per-operation Outcomes mapping here, the new execution.Attempted += op.Count sum, and the report line in FinalStats.String all untested.
That is the half of the PR the user story is about ("per operation, because the total cannot tell you where"), and this mapping is exactly the kind that fails silently: a transposed Expired/DroppedAtCap, or an Attempted left unset, still compiles and still prints a plausible-looking percentage. A test in the style of TestOperationReportOrderIsStable that records a couple of operations plus outcomes on the collector, builds with a Some(...) inclusion summary, and asserts the per-op ledger and Execution.Attempted would pin it.
1 participant
TOT-005 through TOT-008. Sixth in the outcome-tracking stack. Closes User Story 2.
Both ratios, never one
Reporting one hides whichever layer it divides away.
Over offered answers what the profile asked for. A run whose endpoint refuses half shows a halved number, rather than a healthy one measured over the survivors.
Over accepted isolates chain execution, which is the number to read when the RPC layer is not what is under test.
The gap between them is exactly the endpoint's rejection share, so the report names that too.
An unobserved transaction lowers the ratio
It counts in the denominator and never in the numerator. Both alternatives read worse: counting it in the numerator reports a success nobody saw, and dropping it from the denominator reports a healthy ratio over the survivors — the same defect this feature removes one layer up. The separate
status_unavailablecount is what lets a reader tell a low ratio caused by the chain from one caused by the run.Per operation, because the total cannot tell you where
That run reports 46% goodput at the top. A revert rate spread evenly across operations points at the chain; one concentrated in a single call points at the workload — a missing allowance, a wrong ABI, a bad address. The run-level total cannot separate them, and this tool is run ad-hoc against a devnet where the report is the only surface.
It prints as a second line rather than widening the existing one, which is already at its readable limit, so anything parsing that format keeps working.
Already done in the stack
T024 and T027 were satisfied by #69:
TestOutcomesCarryTheOperationpins that outcomes arrive keyed by scenario and operation, not by scenario alone.Checked
Every guard broken on purpose:
Rejected()underflows on an inverted pairgofmt,go vet, full suite clean.